Conversation
| %keyword% Inner | ||
| { | ||
| } | ||
| }".Replace("%keyword%", keyword); |
There was a problem hiding this comment.
Use string interpolation in all tests. This %keyword% notation is really confusing
There was a problem hiding this comment.
@DoctorKrolic , Should I change to Interpolation?
$@"class Outer
{{
$$
{ keyword } Inner
{{
}}
}}";There was a problem hiding this comment.
Yes, I think it looks if not better then at least more familiar. If you want to improve it even more use raw string literals (I would preffer this).
There was a problem hiding this comment.
@DoctorKrolic I pushed modified code.
$$"""
class Outer
{
$$
{{keyword}} Inner
{
}
}
""";| } | ||
|
|
||
| // Move this to ISyntaxFacts if approved. | ||
| protected abstract bool IsConstructableTypeDeclaration(ISyntaxFacts syntaxFacts, SyntaxNode node); |
There was a problem hiding this comment.
Is this necessary? There's already a check in CSharpConstructorSnippetProvider.IsValidSnippetLocationAsync
There was a problem hiding this comment.
@genlu Isn't IsValidSnippetLocationAsync check that the constructor can be placed where 'ctor' is entered?
I think that function is not suitable to get the target class name.
Is it acceptable that the decisions required within AbstractConstructorSnippetProvider depend on the processing implemented in the classes that inherited AbstractConstructorSnippetProvider?
I can't think of any other way to determine the constructable type from within an abstract class.
There was a problem hiding this comment.
You are right, IsValidSnippetLocationAsync](https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Snippets/SnippetProviders/AbstractSnippetProvider.cs,73) is used to decide whether a snippet should placed at the location. So if the check in IsValidSnippetLocationAsync fails, we won't even show the snippet thus it will never get to the point where code change is being calculated.
So to answer your question: yes, it's not only acceptable, it's actually supposed to work this way by design.
| { | ||
| return syntaxFacts.IsTypeDeclaration(node) | ||
| && IsConstructableTypeDeclaration(syntaxFacts, node) | ||
| && node.Span.Contains(position); |
There was a problem hiding this comment.
I think the Contains check would work. But we have FindTokenOnLeftOfPosition you could use for this.
There was a problem hiding this comment.
@genlu Could you please explain a little more how to use FindTokenOnLeftPosition instead of Span.Contains?
In the following pattern, I tried, but I get a CloseBraceToken of InnerClass
$$"""
class Outer
{
class Inner
{
}
$$
}
""";There was a problem hiding this comment.
This helper is what I had in mind. But it is C# specific.
Then there's this, but it seems the behavior might be the same as the code that causing problem in the first place. maybe try to change the code in CSharpSyntaxFacts.GetContainingTypeDeclaration to do something similar to GetContainingTypeOrEnumDeclarations and see what breaks?
There was a problem hiding this comment.
@genlu I changed it to process the parent node if CloseBraceToken is found, instead of using Span.Contains.
I tried using semantic model, but gave up on it because it only fails if the ctor is entered at the end of the file.
Fixes #68176
Fixes AB#1818584
Ignore the input if it is entered in the trivia position of the inner class.
Add Test code.